Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate complete bindings to libzmq #232

Merged
merged 7 commits into from
Jul 17, 2024
Merged

Generate complete bindings to libzmq #232

merged 7 commits into from
Jul 17, 2024

Conversation

JamesWrigley
Copy link
Member

@JamesWrigley JamesWrigley commented May 20, 2024

This is a rather invasive change to use Clang.jl to generate bindings for everything in zmq.h, and then use those bindings in the rest of the library. My motivation for doing this is to make it easier to call the libzmq functions, both for us and for anyone who wants to use the low-level functions (e.g. for things we haven't explicitly wrapped). It's also nice to have the constants generated for us.

It touches a lot of internals so this would definitely benefit from a review from someone more familiar with them 😅 In particular the code for _Message and Message, which took me a while to understand. I've written down my understanding of how that works in the comments of gen.jl. For the reviewers: I've tried to keep the commits atomic, so it might be easiest to review the commits one-by-one. I don't think it's quite ready to merge yet because I'd like to add some more docs, but it should be ready for review.

CC @stevengj, @vtjnash

@JamesWrigley JamesWrigley self-assigned this May 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 49.00990% with 103 lines in your changes missing coverage. Please review.

Project coverage is 74.90%. Comparing base (4e0e343) to head (cd4473b).

Files Patch % Lines
src/bindings.jl 29.85% 94 Missing ⚠️
src/msg_bindings.jl 71.42% 8 Missing ⚠️
src/comm.jl 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #232       +/-   ##
===========================================
- Coverage   91.82%   74.90%   -16.93%     
===========================================
  Files           9       11        +2     
  Lines         367      526      +159     
===========================================
+ Hits          337      394       +57     
- Misses         30      132      +102     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevengj
Copy link
Contributor

Overall, I'm in favor of this, though I haven't had a chance to review it in detail.

@JamesWrigley
Copy link
Member Author

JamesWrigley commented May 21, 2024

I made another change to the internals in 56d3a9b to force using Ref{_Message} everywhere. I think this is correct because it would be unsafe to use most/all of those functions with a plain _Message since it's a immutable struct and might not have a stable address.

@JamesWrigley
Copy link
Member Author

Added docs in 6729eba, and fixed the use of a deprecated function in ef30b30. Pending review, I think this is ready to merge 🤞

(P.S., please don't merge it with the fixup commits, I'll rebase beforehand)

@JamesWrigley
Copy link
Member Author

Rebased the branch, and modified the generated bindings to use ccall() instead of @ccall in c13987d (for compatibility with Julia 1.3).

@JamesWrigley
Copy link
Member Author

(bump)

@stevengj stevengj merged commit e66c836 into master Jul 17, 2024
9 checks passed
@stevengj stevengj deleted the full-bindings branch July 17, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants